Refactor: ingester changes #1574

MarceloRobert · 2025-10-15T19:07:31Z

Changes

Refactors ingester code:

Moves constants to a specific file (also makes constants able to be changed from env vars)
Moves some functions to more specific files
Adds more typing indicators
Lowers the complexity of the most complex functions

How to test

The behavior should be the same as before. So:

Start the ingester
- Create a temporary folder for the submissions as backend/kernelCI_app/management/commands/tmp_submissions (you can change the path, just update the command below)
- Make sure you have your trees-name.yml file somewhere. It usually is on /backend/volume_data as a result of the treeproof command. If you are feeling lazy and you don't want to run the command, there's an example file here (Google Drive link)
- Make sure you have a couple of example submission files for testing. You can grab some in this zip file here (Google Drive link)
- Export the environment variables in your terminal, pointing one of the databases to the local one. Make sure the local one is set as the default one with USE_DASHBOARD_DB=True
- Now you'll be able to run the command with poetry run python3 ./manage.py monitor_submissions --spool-dir kernelCI_app/management/commands/tmp_submissions --trees-file volume_data/trees-name.yaml (being in the /backend directory, update the paths if necessary)
Insert submissions in the folder and check that they are being inserted in the database
You can also test the ingester on docker, make sure the environment variables are on .env.backend and use docker compose run --rm backend before running the command shown above.

Closes #1571

LucasSantos27 · 2025-10-15T20:51:55Z

backend/kernelCI_app/management/commands/helpers/process_submissions.py



-def build_instances_from_submission(data: dict[str, Any]) -> dict[TableNames, list]:
+def build_instances_from_submission(data: dict[str, Any]) -> ProcessedSubmission:


Is there a way to avoid using Any? Maybe with generic

this data is the submission json, so the Any means any value that an item can have: str, int, list, dict, none... IMO in this case it is ok to leave it as Any (and other similar cases around these functions too). This use will at least define that they dict keys are str, otherwise we would be using just dict which just means dict[Any, Any]

LucasSantos27 · 2025-10-15T20:53:45Z

backend/kernelCI_app/management/commands/helpers/kcidbng_ingester.py

-STORAGE_BASE_URL = os.environ.get(
-    "STORAGE_BASE_URL", "https://files-staging.kernelci.org"
-)
+CONVERT_LOG_EXCERPT = False  # If True, convert log_excerpt to output_files url


why not an env as well?

Everything was defined statically in the file, I just added verbose as an env var. I'll make the new constants file and I can make them all env vars 👍

LucasSantos27 · 2025-10-15T20:54:46Z

backend/kernelCI_app/management/commands/helpers/kcidbng_ingester.py

-)
+CONVERT_LOG_EXCERPT = False  # If True, convert log_excerpt to output_files url

 TREES_FILE = "/app/trees.yaml"


what do you think a new file:

constants/ingester.py?

sounds good

LucasSantos27 · 2025-10-15T20:56:26Z

backend/kernelCI_app/management/commands/helpers/log_excerpt_utils.py

+    if VERBOSE:
+        logger.info("Uploading logexcerpt for %s to %s", id, upload_url)


idk this VERBOSE variable could be more descriptive

You mean the description for the VERBOSE variable or the string in this specific logger.info?

LucasSantos27 · 2025-10-15T20:56:49Z

backend/kernelCI_app/management/commands/helpers/log_excerpt_utils.py

+        str: On success upload: the reference url. On failed upload: the original logexcerpt
+    """
+
+    upload_url = f"{STORAGE_BASE_URL}/upload"


outside of function

nuclearcat · 2025-10-24T04:23:25Z

backend/kernelCI_app/helpers/logger.py

+    """Write debug/perf output to stdout. Logger was unreliable in some environments."""
+    try:
+        print(
+            f"[{time.strftime("%Y-%m-%d %H:%M:%S", time.localtime())}] {msg}",


Nested double quotes on f-string

nuclearcat · 2025-10-24T04:25:58Z

backend/kernelCI_app/management/commands/helpers/file_utils.py

+        if os.path.isfile(os.path.join(spool_dir, f)) and f.endswith(".json")
+    ]
+    if not json_files:
+        return


Is it ok to return like this instead of tuple? I think we should return empty list and 0 files
return [], 0 ?

Thanks, this simple return should be for the ingest_submissions_parallel, so I returned the [], 0 here and added another condition there

Changed with the rebase, not needed anymore

nuclearcat · 2025-10-24T04:26:42Z

backend/kernelCI_app/management/commands/helpers/kcidbng_ingester.py

-        )
-    )
+    json_files, total_bytes = get_spool_files(spool_dir)



if not json_files: maybe? i there is no spool files, we might need to put small delay and check again

Yeah, I had moved the if not json_files to the other function but it should be here. I'll fix it

nuclearcat · 2025-10-24T04:27:19Z

backend/kernelCI_app/management/commands/helpers/kcidbng_ingester.py

+    """
+    Consumes the list of objects and tries to insert them into the database.
+    """
+    total = len(issues_buf)


I think should be something like:

total = ( len(issues_buf) + len(checkouts_buf) + len(builds_buf) + len(tests_buf) + len(incidents_buf) )

nuclearcat · 2025-10-24T04:28:41Z

backend/kernelCI_app/typeModels/modelTypes.py

+from kernelCI_app.models import Builds, Checkouts, Incidents, Issues, Tests
+
+type TableNames = Literal["issues", "checkouts", "builds", "tests", "incidents"]
+type TableModels = Issues | Checkouts | Builds | Tests | Incidents


I am not sure in this part...
TableModels = Issues | Checkouts | Builds | Tests | Incidents denotes instances, but MODEL_MAP stores classes and consume_buffer expects a class

does it denote instances? IIUC it denotes the classes themselves, hovering over the TableModels variable I see (type) TableModels = type[Issues] | type[Checkouts]... directly

nuclearcat · 2025-10-24T04:29:51Z

backend/kernelCI_app/constants/ingester.py

+    INGEST_FLUSH_TIMEOUT_SEC = float(os.environ.get("INGEST_FLUSH_TIMEOUT_SEC", 2.0))
+except (ValueError, TypeError):
+    logger.warning("Invalid INGEST_FLUSH_TIMEOUT_SEC, using default 2.0")
+    INGEST_FLUSH_TIMEOUT_SEC = 5.0


If default is 2.0, we must set 2.0 here too

nuclearcat · 2025-10-24T04:31:52Z

backend/kernelCI_app/typeModels/modelTypes.py

+from typing import Literal
+from kernelCI_app.models import Builds, Checkouts, Incidents, Issues, Tests
+
+type TableNames = Literal["issues", "checkouts", "builds", "tests", "incidents"]


This require Python >=3.12 (PEP 695). Do we use 3.12 and above?

We do, it's set on pypoetry.toml and we have been using this format for a while

Even with this reduction, it's still hard to lower the complexity even further without creating idempotent functions Closes kernelci#1571

MarceloRobert self-assigned this Oct 15, 2025

LucasSantos27 reviewed Oct 15, 2025

View reviewed changes

MarceloRobert force-pushed the refactor/ingester branch from fa7db92 to 1255402 Compare October 16, 2025 20:26

MarceloRobert changed the title ~~WIP: Refactor: ingester changes~~ Refactor: ingester changes Oct 16, 2025

MarceloRobert marked this pull request as ready for review October 16, 2025 20:32

MarceloRobert requested a review from nuclearcat October 22, 2025 13:26

nuclearcat requested changes Oct 24, 2025

View reviewed changes

MarceloRobert force-pushed the refactor/ingester branch 2 times, most recently from acc8bd1 to 40ba06b Compare October 29, 2025 19:58

MarceloRobert requested a review from nuclearcat October 30, 2025 13:13

MarceloRobert added 7 commits October 30, 2025 16:21

refactor: improve typing and buffer consumption

323f09a

feat: add ingester constants

a3b2fd3

refactor: move log_excerpt related functions

fe61e8f

refactor: move out to logger

dc6373d

refactor: move ingester file functions

bc0e956

refactor: improve ingester typing

98607ff

refactor: lower complexity of db_worker

6dab5dc

Even with this reduction, it's still hard to lower the complexity even further without creating idempotent functions Closes kernelci#1571

MarceloRobert force-pushed the refactor/ingester branch from 40ba06b to 6dab5dc Compare October 30, 2025 19:21

This was referenced Oct 31, 2025

Test: ingester file functions unit tests #1592

Open

Test: ingester excerpt #1593

Open



		def build_instances_from_submission(data: dict[str, Any]) -> dict[TableNames, list]:
		def build_instances_from_submission(data: dict[str, Any]) -> ProcessedSubmission:

		if VERBOSE:
		logger.info("Uploading logexcerpt for %s to %s", id, upload_url)

Refactor: ingester changes #1574

Are you sure you want to change the base?

Refactor: ingester changes #1574

Conversation

MarceloRobert commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

How to test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MarceloRobert commented Oct 15, 2025 •

edited

Loading